1416 add support for mfa for tom user logins - #1653
Conversation
These docs describe the result django-allauth integration which will be TOM Toolkit 3.1.0
…fa-for-tom-user-logins
django-allauth now answers every /accounts/ page replacing Django's plain LoginView/LogoutView.
This commit deserves a thorough explanation (see also the docstring
in tom_common.adapters): tom_common.adapters is the "customisation
seam" that the docs refer to: a TOM changes account behavior by
subclassing these adapters and re-pointing the configuration variables
like ACCOUNT_ADAPTER / MFA_ADAPTER from settings.py (for example,
re-pointing them to subclasses in `custom_code`).
- TomAccountAdapter.is_open_for_signup: allauth's default is OPEN
self-registration, which the URL cutover briefly inherited; signup is
now closed unless TOM_REGISTRATION_STRATEGY says otherwise (the
strategies themselves land with the registration work).
- TomMFAAdapter: TOTP secrets and recovery-code seeds are stored
encrypted with the SECRET_KEY-derived cipher rather than allauth's
plaintext-in-JSON default, so authenticator data participates in the
same key-rotation story as every EncryptedModelField; the
issuer (the name that appears in authenticator apps) is TOM_NAME;
and can_delete_authenticator enforces the TOM_MFA_REQUIRED policy
(users under the policy cannot remove their second factor —
an administrator still can, in the admin).
- rotate_encryption_key also re-encrypts Authenticator JSON data
('secret', 'seed', 'migrated_codes'), through the adapter so a TOM
with a custom MFA_ADAPTER rotates by its own scheme.
all these template overrides are about getting the django-allauth templates to look like TOM Toolkit. Some specific details worth mentioning: - tom_common/base.html: the new `content_container` block nesting the existing content block distinguished the TOM Toolkit content block from the allauth block that is also named 'content'. A layout that wants to wrap page content needs a block of its own and any plugin layout can use it the same way.
Previously, under AUTH_STRATEGY='LOCKED' the middleware admitted anonymous users only to the login path and OPEN_URLS. This adds the necessary django-allauth URL names to the list. These are the pages that the anonymous users must visit to become not anonymous.
do urlpattern surgery accordingly
When htmx receives a 302 it follows the redirect and swaps the destination page into the requesting fragment — so any authentication redirect fired during an HTMX action (session expiry, the 300 s reauthentication window, a LOCKED bounce) rendered the login page inside a table cell (bad). HTMXRedirectMiddleware rewrites such responses to HX-Redirect, causing HTMX to navigate the whole browser window (good).
The alternative was having this stuff scattered in modules all over tom_common. Now it's in one place. (middleware and signals remain in those modules).
settings introduced: (TOM_MFA_REQUIRED, TOM_PASSWORD_EXPIRY_DAYS, and TOM_REQUIRED_USER_FIELDS). AccountRequirementsMiddleware runs checks for every logged-in request and redirects to the first unmet check's page, with a message naming what to do there. Checks implemented in accounts/requirements.py -- see docstring.
Setting a version string is the signal that invokes the check and makes every user accept the terms before using the TOM.
Allows Administrators to see the requirement status of each user. Also: guardian's AnonymousUser is excluded from the user list. It is a permissions sentinel from django-guardian, not a person; filtered on guardian's ANONYMOUS_USER_NAME setting.
Events and where they are caught: - login success/failure/logout: Django's auth signals. - two-factor authenticator added/removed/reset: allauth's mfa signals. - API-token regeneration: in the view, recording both the token's owner and the acting user (they differ when a superuser regenerates).
This includes the DRF API in the configured authentication requirements. TomTokenAuthentication replaces the REST framework's TokenAuthentication in REST_FRAMEWORK. With neither TOM_API_TOKEN_REQUIRES_MFA nor TOM_API_TOKEN_EXPRIRY_DAYS setting configured, it behaves identically. So, existing TOMs see no change.
With TOM_API_TOKEN_REQUIRES_MFA a token can now only come into existence through a channel protected by the second factor. - api/token-auth/ is served by TomObtainAuthToken, which refuses (403, pointing at the profile page) under the flag. - Token regeneration under the flag is owner-only and requires recent reauthentication via allauth's page. - Raise403Middleware no longer converts /api/ 403s into browser login redirects so scripts can see the 403 JSON response. - The token partial shows created/expires dates (token_expiry_date helper) - The REST API docs name TomTokenAuthentication and the TOM_API_TOKEN_* settings.
The administrator's half of approval_required registration.
|
@rachel3834 The checklist above represents a reasonable tour of the registration/authorization configuration options. |
|
I went through the whole checklist and had no problems! |
|
I have been through almost the entire checklist (only bit skipped was the configuration of actual email backend). |
|
Went through the checklist. Used a local authenticator - no way I was pulling out my phone that many times. I feel like this could have been 3-4 pull requests, easily. A lot of this functionality is tied to django-allauth, but it could have been implemented gradually. |
Fingel
left a comment
Fingel
left a comment
There was a problem hiding this comment.
I can't really review this code in a non-superficial way, there's just too much of it and it all looks plausible (obviously, it works).
I will say I'm very skeptical of the 1,000+ lines of tests. Django-allauth already has a test suite so I'm not sure we need tests to cover functionality that isn't provided by us.
For example:
def test_login_path_is_served_by_allauth(self):
"""allauth is mounted before the plugin loop and the aliases, so its view answers the path."""
self.assertEqual(resolve(reverse('login')).url_name, 'account_login')All lot of these are just testing configuration or Allauth behavior. There is a cost here as the test suite grows and continues to become slower.
jchate6
left a comment
jchate6
left a comment
There was a problem hiding this comment.
I've barely touched the code, and didn't even finish the docs.
Documentation
I've made a bunch of comments, and things I think can be changed, but I feel like a lot of my comments could be extrapolated to much of the rest of the documentation at least. Docs have different purposes and different intended readers. Much of the documentation in this PR is not useful.
Documentation intended for users needs to be detailed, but well organized. They should never see a wall of sentence fragments pulled into bullets, or pages of incomprehensible tables. Tables are for looking up specific information, not outlining a procedure. Tables should always have titles and often a description of what they are for, and foot notes describing complexity.
Documentation intended for developers should be short, concise, and assume a basic familiarity with both the toolkit and its docs, but also the coding language. If an in-line comment cannot be written in 2 lines, we should seriously reconsider the code itself.
Look at most of the documentation you have previously written within the TOMToolkit for examples.
Code
Beyond the documentation, I have not reviewed the code. It is not possible for me to give a code review in the time available, and I'm worried it would take significantly more effort to review this code than was put into creating it.
From the limited experimentation I have done, registration functions, and 2fa works, even if it is very clunky. I have seen no documentation on how to add, implement, or retain an email backend like the one described in TOM registration. This is key functionality for users of that package.
Most of the actual templates seem to be from django-allauth. But seem quite limited and should probably be customized for TOM use.
This PR is entirely too long and complex, adding too many new features to be properly reviewed. I think this PR should be broken down into parts, each adding new discrete functionality that slowly builds out these changes so that each step can be tested, validated, and cleaned up. I don't really see a way to approve it otherwise.
| 'allauth.account.auth_backends.AuthenticationBackend', # show "pending approval" for inactive accounts | ||
| 'guardian.backends.ObjectPermissionBackend', | ||
| ) | ||
| AUTHENTICATION_BACKENDS = TOMTOOLKIT_AUTHENTICATION_BACKENDS |
There was a problem hiding this comment.
This is redundant with what is in settings.tmpl.
| CRISPY_ALLOWED_TEMPLATE_PACKS = "bootstrap5" | ||
|
|
||
| # django-allauth configuration. | ||
| ACCOUNT_ADAPTER = 'tom_common.accounts.adapters.TomAccountAdapter' |
There was a problem hiding this comment.
This should include a link to the django-allauth documentation. Without that context most of these comments are meaningless.
| ACCOUNT_SIGNUP_FIELDS = ['username*', 'email*', 'password1*', 'password2*'] | ||
| ACCOUNT_EMAIL_VERIFICATION = 'none' | ||
| ACCOUNT_AUTHENTICATED_LOGIN_REDIRECTS = False # prevent redirect loops; redirect to login with message | ||
| MFA_SUPPORTED_TYPES = ['totp', 'recovery_codes'] # passkeys/WebAuthn deliberately not enabled |
| ACCOUNT_AUTHENTICATED_LOGIN_REDIRECTS = False # prevent redirect loops; redirect to login with message | ||
| MFA_SUPPORTED_TYPES = ['totp', 'recovery_codes'] # passkeys/WebAuthn deliberately not enabled | ||
| MFA_ALLOW_UNVERIFIED_EMAIL = True | ||
| MFA_TOTP_TOLERANCE = 1 # accept codes from the adjacent 30 s window (clock skew) |
There was a problem hiding this comment.
This comment doesn't mean anything.
We either need to include more context for parameters that might be changed or overwritten by users, or just link to the relevant documentation and let developers decide what they need to update based on that.
| 'django.contrib.auth.backends.ModelBackend', | ||
| 'guardian.backends.ObjectPermissionBackend', | ||
| ) | ||
| AUTHENTICATION_BACKENDS = TOMTOOLKIT_AUTHENTICATION_BACKENDS |
There was a problem hiding this comment.
If there is any chance that a user might want to include their own authentication backends, we should include that option here like we do with installed_apps above.
| {# djlint:on #} | ||
| {% endelement %} | ||
| {% endif %} | ||
| {% if can_download_codes %} |
There was a problem hiding this comment.
Under what circumstances is can_download_codes true?
| {% translate "I have saved my recovery codes" %} | ||
| {% endslot %} | ||
| {% endelement %} | ||
| {% endif %} |
There was a problem hiding this comment.
There should be a "back" button.
Feel free to have it only activated when the "saved codes" box is checked.
| password** (TOTP) from an authenticator app — Microsoft Authenticator, Google Authenticator, Authy, 1Password, | ||
| Bitwarden and similar all work — plus a set of **recovery codes** for when the app is unavailable. | ||
|
|
||
| For users |
There was a problem hiding this comment.
This section is largely useless here. This all needs to be clearly displayed and described on the actual page the user sees when activating 2FA.
The existing page doesn't explain what 2FA is, why it's important, what an authenticator app is, or how to use one. MOST people do not use authenticator apps for 2FA, they use an email or phone number. You do not explain how to use the recovery codes You do not explain why they are needed or why someone should hold on to them.
Furthermore, when a user cannot log in because of 2FA they have no recourse. First they should be told to use a recover code (and that that code will no longer be available. If that doesn't work, they should be directed to a manager's email if available and told to contact the tom admin otherwise.
| password** (TOTP) from an authenticator app — Microsoft Authenticator, Google Authenticator, Authy, 1Password, | ||
| Bitwarden and similar all work — plus a set of **recovery codes** for when the app is unavailable. | ||
|
|
||
| For users |
There was a problem hiding this comment.
Rename this to "From the User's Perspective"
The TOMToolkit docs are not for users. That's like expecting someone who needs help with submitting and observation on the LCO website to be directed to the astropy docs for help.
| If you lose both the app and the recovery codes, an administrator can remove your authenticator in the Django admin | ||
| (*Multi-factor authentication* → *Authenticators*); you then enrol again. | ||
|
|
||
| For TOM developers |
There was a problem hiding this comment.
From a Developer's perspective
5 participants
Suggested reviewing path:
==========================================
Do this / See that
Each part adds settings on top of the previous part's.
Prerequisites
python manage.py migrate),runserverup (steps below say :8274 — substitute your port).EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend'and
MANAGERS = [('You', 'you@example.com')].adminsuperuser (password known). An authenticator app. A second browser orincognito window ("window B").
curl.Part 0 — baseline (no TOM_* settings configured)
/accounts/login/→ styled login page; no Register button, no "sign up" line, no "Forgot your password?"./accounts/signup/→ "Sign Up Closed" page, tells you to contact the administrators./admin/login/→ redirected to the TOM login page./api-auth/login/→ redirected to the TOM login page.Part 1 — enroll and use 2FA (still no settings)
(no Download button under the default show-once policy; clicking the codes copies them).
without confirming that you have saved your recovery codes?…". Cancel stays; checking the box
then leaving asks nothing. (Closing the tab instead shows the browser's own generic warning.)
/accounts/2fa/→ cards: Authenticator App "active" (Deactivate button), Recovery Codes "10 of 10"./accounts/2fa/now says "9 of 10".tester(with an email address; no 2FA yet).Part 2 — require 2FA
/accounts/2fa/totp/deactivate/→ no Deactivate button; explanation + "Back to two-factor settings" link.Part 3 — terms of service
/terms/accept/, message says accept to continue; placeholder text tells the operator where to write terms.templates/tom_common/partials/terms_of_service_text.htmlin the TOM with your terms → accept page shows them./terms/in a logged-out window → public read-only terms page.(until tester accepts in window B).
'v2'⟳ → prompted again; accept; column header now says (v2)./admin/→ Terms of service acceptances: rows for v1 and v2 with timestamp and IP; read-only.Part 4 — password rules and expiry
alllowercase→ rejected, missing classes listed by name.Part 5 — required profile fields
Part 6 — API tokens
curl -H "Authorization: Token <your-token>" http://localhost:8274/api/targets/→ 200 JSON.curl --request POST http://localhost:8274/api/token-auth/ -H "Content-Type: application/json" -d '{"username":"admin","password":"<pw>"}'→ 403, message points at the profile page.Part 7 — open registration
newuser: form shows username/email/password/name/organization/phone + terms checkbox;the five required fields are marked; "terms of service" in the checkbox label links to
/terms/;submitting without the checkbox → error.
/admin/→ newuser is in the Public group.Part 8 — approval-required registration
applicant→ not logged in; runserver console prints the awaiting-approval mail to MANAGERS.EMAIL_HOST = 'localhost'with the SMTP backend ⟳ →Pending users table warns email is not configured; approving warns you to tell the user directly;
manage.py checkreportstom_common.W002. Restore the console backend ⟳ afterwards.Part 9 — LOCKED + password reset
Part 10 — security log (optional)
Wrap up
python manage.py check→ no warnings (tom_common.W001/W002would name anytoken-authentication or email misconfiguration).